Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor: Simplify the mobile menu files #770

Merged
merged 2 commits into from
Feb 25, 2020
Merged

Conversation

laurelfulford
Copy link
Contributor

All Submissions:

Changes proposed in this Pull Request:

In anticipation of potentially adding more menus (#766), this PR simplifies the mobile menu code. Rather than splitting it over two files (AMP and non-AMP), it combines them into one with some conditional checks before outputting the AMP-specific code. There's enough of a significant code overlap, with potentially more to come, that this seems easier to manage.

How to test the changes in this Pull Request:

  1. Apply the PR.
  2. Make sure you don't have a short header set, and that you have menus assigned to the four locations in the header (Primary, Secondary, Tertiary and Social).
  3. With AMP on, confirm all these menus display as expected.
  4. Shrink down the browser to a mobile-sized screen, and confirm that the menu slideout works.
  5. Turn off AMP.
  6. Retest the mobile menu and confirm it still works.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@laurelfulford laurelfulford added the [Status] Needs Review The issue or pull request needs to be reviewed label Feb 11, 2020
@jeffersonrabb jeffersonrabb self-requested a review February 24, 2020 23:12
Copy link
Contributor

@jeffersonrabb jeffersonrabb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

newspack-theme/template-parts/header/mobile-sidebar.php Outdated Show resolved Hide resolved
@jeffersonrabb jeffersonrabb added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Feb 24, 2020
@laurelfulford
Copy link
Contributor Author

Thanks Jeff! I've fixed the code comment so it actually makes sense :D

@laurelfulford laurelfulford merged commit 43f653d into master Feb 25, 2020
@laurelfulford laurelfulford deleted the simplify/mobile-menu branch February 25, 2020 04:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Approved The pull request has been reviewed and is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants